Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(uiState): add ui-state directive #1932

Closed
wants to merge 1 commit into from
Closed

feat(uiState): add ui-state directive #1932

wants to merge 1 commit into from

Conversation

kasperlewau
Copy link
Contributor

ui-sref with dynamic state/option/param pointers.

Usage:

<a ui-state="state" ui-state-params="params" ui-state-opts="opts">
  Link
</a>
$scope.state = 'myStateName';
$scope.params = { id: 5 };
$scope.opts = { reload: true };

The resulting href would be pointed to myStateName({ id: 5 }) and $state.go would be called with reload: true.


It's also possible to specify the params straight into the ui-state directive as such:

<a ui-state="stateWithParam"></a>
$scope.stateWithParam = 'myStateName({ id: 10 })';

I started off writing this as a completely standalone directive, but given that it is desirable for it to work much like ui-sref, I felt there was too much duplication going on. ui-state can therefore be seen as a wrapper around the behaviour of ui-sref, nothing more to it really.

Now, this thing is heavy on the $eval and slightly so on the $watch side of things, so if someone has any nifty ideas on how to make that performant (I have not tested the performance aspects of ui-state in depth), that'd be super sweet.


Do let me know if you think there's a test missing among the added ones - I can't say I'm 100% certain that I've tested every aspect of a 'dynamic ui-sref directive'.

Reference issues: #395 #900
Suggested implementation

* @restrict A
*
* @description
* Much like ui-sref, but will accept named $scope propertys to evaluate for a state definition,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/propertys/properties/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get on it asap, thanks for the catch :)

@nateabele
Copy link
Contributor

Hey, thanks for pulling this together! I made a couple of comments, but barring those it looks good to me. @christopherthielen Any comments?

@christopherthielen christopherthielen self-assigned this May 17, 2015
@christopherthielen
Copy link
Contributor

I like the effort to reuse the existing ui-sref code for ui-sref-active, (hopefully that works). I'll have to play with this for a while before I can comment on the implementation.

@kasperlewau
Copy link
Contributor Author

Hmph. No sauce. evaluating the full array will always return undefined.

I can think of a couple of ways to achieve the expected result and move away from concatenation:

  return [scope.$eval(attrs.uiState), scope.$eval(attrs.uiStateParams), scope.$eval(attrs.uiStateOpts)];

  return [scope[attrs.uiState], scope[attrs.uiStateParams], scope[attrs.uiStateOpts]];

  return [attrs.uiState, attrs.uiStateParams, attrs.uiStateOpts].map(function (prop) {
    return scope.$eval(prop);
  });  

thoughts?

@nateabele
Copy link
Contributor

Hmph. No sauce. evaluating the full array will always return undefined.

Yeah, I just realized I was looking at it wrong... hmm...

I can think of a couple of ways to achieve the expected result and move away from concatenation:

I'm not so much worried about the syntax as I am about the performance. Trying to think of how we can get it down to one $watch() / $eval(). Those are going to add up really quickly. I wonder if attrs.$observe() would be any cheaper...

I'll test it out when I have time, unless you beat me to it.

@kasperlewau
Copy link
Contributor Author

I'll see if I can come up with some way of cutting it down to a single $eval, not sure what that would look like though.

Regarding attrs.$observe vs scope.$watch - I do believe that the former is slightly faster, but I'm not 100% on that one. In any case I think the difference would be negligible, it's the amount of $eval's that are worrying.

@kasperlewau
Copy link
Contributor Author

Reverted back to using 3x $eval, so as to get a passing build. Still haven't figured out a better way to evaluate the three values without three consecutive calls to $eval or $parse (albeit I haven't worked on this full time...).

Works much like ui-sref but with a slight difference in that it accepts
named $scope properties to evaulate for a state ref.

Closes #395
@kasperlewau
Copy link
Contributor Author

Unfortunately I have still not been able to figure out a way to run a single $eval call for three separate expressions - without parsing possible objects into JSON and then concatenating the strings.

I did clean up the code ever so slightly in the $watch fn expression from:

return [
  scope.$eval(attrs.uiState),
  scope.$eval(attrs.uiStateParams),
  scope.$eval(attrs.uiStateOpts)
];

Into:

return [
  attrs.uiState,
  attrs.uiStateParams,
  attrs.uiStateOpts
].map(scope.$eval.bind(scope));

Opted for angular.bind so as to get a passing spec on TravisCI (all passing locally with fn.proto.bind running karma integrate).

One could argue that it "helps" with ES5+IE8, but that is sort of moot seeing as how .map is being utilised.

@kasperlewau
Copy link
Contributor Author

@nateabele @christopherthielen - What are your thoughts on the current state of things in this PR? Have you gotten a chance to play around with it?


Personally, I will probably never use ui-state (it was always just about building it for me), so I'm not in a rush. Though I have been receiving some questions as to the status of the PR and when a possible merge would occur - I'm sort of at a loss on what to respond with (barring the ah, just +1 it friend).

I do share your concern in regards to multiple calls to $eval, and I have yet (read to test out the $attrs.observe approach. Even so I'm not crystal clear on how one would measure the possible difference in performance, any and all help in figuring that one out would be very appreciated.

nateabele added a commit that referenced this pull request Aug 19, 2015
 - Refactor StateRefDirective for better modularity
 - Drop key restrictions on ui-sref-opts
 - Improves performance over prior implementation with no extra $eval()’s

Fixes #395, #900, #1932
nateabele added a commit that referenced this pull request Aug 19, 2015
 - Refactor StateRefDirective for better modularity
 - Drop key restrictions on ui-sref-opts
 - Improves performance over prior implementation with no extra $eval()’s

Fixes #395, #900, #1932
@nateabele
Copy link
Contributor

Refactored this into #2187.

@kasperlewau
Copy link
Contributor Author

Closing this in favour of #2187.

ExpFront pushed a commit to ExpFront/ui-router that referenced this pull request Jun 23, 2016
 - Refactor StateRefDirective for better modularity
 - Drop key restrictions on ui-sref-opts
 - Improves performance over prior implementation with no extra $eval()’s

Fixes angular-ui#395, angular-ui#900, angular-ui#1932
@kasperlewau kasperlewau deleted the ui-state-directive branch December 25, 2016 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants